From ddf3c7955a51d251779fee9e971d3d30ed55ebaf Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 8 Jun 2015 15:18:38 -0700 Subject: [PATCH] Fix spurious rebuilds when switching source paths The method of creating package ids in Cargo means that all sub-crates of a main repo have the same package id, which encodes the path it came from. This means that if the "root crate" switches, the package id for all dependencies will change, causing an alteration in package id hashes, causing recompiles. This commit alters a few points of hashing to ensure that whenever a package is being hashed for freshness the *source root* of the crate is used instead of the root of the main crate. This cause the hashes to be consistent across compiles, regardless of the root package. Closes #1694 --- src/cargo/core/package.rs | 23 ++++++++------ src/cargo/core/package_id.rs | 12 +++++--- src/cargo/ops/cargo_rustc/context.rs | 2 +- src/cargo/ops/cargo_rustc/layout.rs | 2 +- src/cargo/util/toml.rs | 6 ++-- tests/test_cargo_compile_path_deps.rs | 44 +++++++++++++++++++++++++++ 6 files changed, 71 insertions(+), 18 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 593190162..f71f5025e 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -4,14 +4,7 @@ use std::slice; use std::path::{Path, PathBuf}; use semver::Version; -use core::{ - Dependency, - Manifest, - PackageId, - Registry, - Target, - Summary, -}; +use core::{Dependency, Manifest, PackageId, Registry, Target, Summary, Metadata}; use core::dependency::SerializedDependency; use util::{CargoResult, graph}; use rustc_serialize::{Encoder,Encodable}; @@ -78,6 +71,10 @@ impl Package { pub fn has_custom_build(&self) -> bool { self.targets().iter().any(|t| t.is_custom_build()) } + + pub fn generate_metadata(&self) -> Metadata { + self.package_id().generate_metadata(self.root()) + } } impl fmt::Display for Package { @@ -96,7 +93,15 @@ impl Eq for Package {} impl hash::Hash for Package { fn hash(&self, into: &mut H) { - self.package_id().hash(into) + // We want to be sure that a path-based package showing up at the same + // location always has the same hash. To that effect we don't hash the + // vanilla package ID if we're a path, but instead feed in our own root + // path. + if self.package_id().source_id().is_path() { + (0, self.root(), self.name(), self.package_id().version()).hash(into) + } else { + (1, self.package_id()).hash(into) + } } } diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index f585a0137..25bfc2796 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -3,6 +3,7 @@ use std::error::Error; use std::fmt::{self, Formatter}; use std::hash::Hash; use std::hash; +use std::path::Path; use std::sync::Arc; use regex::Regex; @@ -135,10 +136,13 @@ impl PackageId { pub fn version(&self) -> &semver::Version { &self.inner.version } pub fn source_id(&self) -> &SourceId { &self.inner.source_id } - pub fn generate_metadata(&self) -> Metadata { - let metadata = short_hash( - &(&self.inner.name, self.inner.version.to_string(), - &self.inner.source_id)); + pub fn generate_metadata(&self, source_root: &Path) -> Metadata { + // See comments in Package::hash for why we have this test + let metadata = if self.inner.source_id.is_path() { + short_hash(&(0, &self.inner.name, &self.inner.version, source_root)) + } else { + short_hash(&(1, self)) + }; let extra_filename = format!("-{}", metadata); Metadata { metadata: metadata, extra_filename: extra_filename } diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index 9ea3dfc0b..4a1284074 100644 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -275,7 +275,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { // Make sure that the name of this test executable doesn't // conflict with a library that has the same name and is // being tested - let mut metadata = pkg.package_id().generate_metadata(); + let mut metadata = pkg.generate_metadata(); metadata.mix(&format!("bin-{}", target.name())); Some(metadata) } else if pkg.package_id() == self.resolve.root() && !profile.test { diff --git a/src/cargo/ops/cargo_rustc/layout.rs b/src/cargo/ops/cargo_rustc/layout.rs index 58c644a82..5ab7bb5a4 100644 --- a/src/cargo/ops/cargo_rustc/layout.rs +++ b/src/cargo/ops/cargo_rustc/layout.rs @@ -130,7 +130,7 @@ impl Layout { } fn pkg_dir(&self, pkg: &Package) -> String { - format!("{}-{}", pkg.name(), short_hash(pkg.package_id())) + format!("{}-{}", pkg.name(), short_hash(pkg)) } } diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 080e0ce34..2bb1a1ff1 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -380,7 +380,7 @@ impl TomlManifest { } let pkgid = try!(project.to_package_id(source_id)); - let metadata = pkgid.generate_metadata(); + let metadata = pkgid.generate_metadata(&layout.root); // If we have no lib at all, use the inferred lib if available // If we have a lib with a path, we're done @@ -427,8 +427,8 @@ impl TomlManifest { for bin in bins.iter() { if blacklist.iter().find(|&x| *x == bin.name) != None { - return Err(human(&format!("the binary target name `{}` is forbidden", - bin.name))); + return Err(human(&format!("the binary target name `{}` is \ + forbidden", bin.name))); } } diff --git a/tests/test_cargo_compile_path_deps.rs b/tests/test_cargo_compile_path_deps.rs index 608a1b3a3..13586953a 100644 --- a/tests/test_cargo_compile_path_deps.rs +++ b/tests/test_cargo_compile_path_deps.rs @@ -749,3 +749,47 @@ test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured ", COMPILING, p.url(), COMPILING, p.url()))); }); + +test!(custom_target_no_rebuild { + let p = project("foo") + .file("Cargo.toml", r#" + [project] + name = "foo" + version = "0.5.0" + authors = [] + [dependencies] + a = { path = "a" } + "#) + .file("src/lib.rs", "") + .file("a/Cargo.toml", r#" + [project] + name = "a" + version = "0.5.0" + authors = [] + "#) + .file("a/src/lib.rs", "") + .file("b/Cargo.toml", r#" + [project] + name = "b" + version = "0.5.0" + authors = [] + [dependencies] + a = { path = "../a" } + "#) + .file("b/src/lib.rs", ""); + p.build(); + assert_that(p.cargo("build"), + execs().with_status(0) + .with_stdout(&format!("\ +{compiling} a v0.5.0 ([..]) +{compiling} foo v0.5.0 ([..]) +", compiling = COMPILING))); + + assert_that(p.cargo("build") + .arg("--manifest-path=b/Cargo.toml") + .env("CARGO_TARGET_DIR", "target"), + execs().with_status(0) + .with_stdout(&format!("\ +{compiling} b v0.5.0 ([..]) +", compiling = COMPILING))); +}); -- 2.30.2